New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Optional emptiness handler #278
Conversation
" void foo() {", | ||
" Optional<Object> a = Optional.of(null);", | ||
" // BUG: Diagnostic contains: dereferenced expression a.get() is @Nullable", | ||
" a.get().toString();", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right; Optional.of(null)
will throw a NullPointerException
because Optional
can't wrap null
. So this a.get()
won't be called, and in other contexts Optional#get
will never yield null
.
Or do I misunderstand completely? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand this does not matter. As we are not going to run this code. We just run the static analysis on this code. So as long as we initialise a
with any method it is alright.
Anyway if you think I can write this test differently. Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that in the context of this test no NPE will happen, but the test does show that the plugin would make a "false statement" in case it were applied to real code. After all, Optional#get()
is never @Nullable
.
A more realistic test could be:
Optional<Object> a = Optional.empty();
// BUG: Diagnostic contains: dereferenced expression a.get() may be empty
a.get().toString();
... but that does require customizing the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙂
I will think about how can I customize the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should actually be modeling Optional.of()
such that the parameter is @NonNull
. We should add a library model here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the actual result of Optional<Object> a = Optional.empty(); a.get()
? An exception?
Actually, thinking about how to customize the message, we could have a library model list of methods with a "pseudo-null" return and the actual message that should be printed instead of the usual "is @nullable" error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the actual result of
Optional<Object> a = Optional.empty(); a.get()
? An exception?
Yep:
$ jshell
| Welcome to JShell -- Version 11.0.2
| For an introduction type: /help intro
jshell> Optional<Object> a = Optional.empty()
a ==> Optional.empty
jshell> a.get()
| Exception java.util.NoSuchElementException: No value present
| at Optional.get (Optional.java:148)
| at (#2:1)
W.r.t. your customization suggestion: SGTM :)
I really like the idea here! A few high-level thoughts:
Bottom line, I like the approach here, but (1) we should almost certainly have it off by default at first, and (2) before we land we should probably test it on some large code base like Uber's to see how many false positives turn up. @lazaroclapp any further thoughts? |
386d747
to
884528b
Compare
@@ -107,6 +107,9 @@ | |||
*/ | |||
boolean acknowledgeRestrictiveAnnotations(); | |||
|
|||
/** @return true if Optional Emptiness Handler is to be used. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be more specific on what this does? We should say this enables checking for Optional.get()
calls without checking Optional.isPresent()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Either more detail here or maybe even a paragraph or two in the docs and link from here (maybe both?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
Will add few lines in the wiki
@@ -1933,7 +1932,17 @@ private Description matchDereference( | |||
return Description.NO_MATCH; | |||
} | |||
if (mayBeNullExpr(state, baseExpression)) { | |||
String message = "dereferenced expression " + baseExpression.toString() + " is @Nullable"; | |||
String message; | |||
if (handler.checkIfOptionalGetCall(baseExpression, state)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want a more general mechanism here, not something specific to Optional.get()
. We should refactor and let the handler return a different error message if desired given the baseExpression
and state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. We need an entry point into the base Handler
interface that allows to override error messages for a given combination of message type (see MessageTypes
), expression, and state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍Done
@@ -107,6 +107,9 @@ | |||
*/ | |||
boolean acknowledgeRestrictiveAnnotations(); | |||
|
|||
/** @return true if Optional Emptiness Handler is to be used. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Either more detail here or maybe even a paragraph or two in the docs and link from here (maybe both?)
@@ -1933,7 +1932,17 @@ private Description matchDereference( | |||
return Description.NO_MATCH; | |||
} | |||
if (mayBeNullExpr(state, baseExpression)) { | |||
String message = "dereferenced expression " + baseExpression.toString() + " is @Nullable"; | |||
String message; | |||
if (handler.checkIfOptionalGetCall(baseExpression, state)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. We need an entry point into the base Handler
interface that allows to override error messages for a given combination of message type (see MessageTypes
), expression, and state.
@@ -169,6 +175,23 @@ public NullnessStore getLocalVarInfoBefore(TreePath path, Context context) { | |||
|| e.getKind().equals(ElementKind.LOCAL_VARIABLE); | |||
} | |||
} | |||
|
|||
// a filter for Optional get() call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding code to the core here? I thought the idea was to abstract this all behind a handler and guard it with a flag. This code is neither... am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it to the handler 👍
* @param state The current visitor state. | ||
* @return If the method call in the expression is to Optional.get(). | ||
*/ | ||
boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, this should be a more general message overriding method.
@@ -289,6 +289,7 @@ private static LibraryModels loadLibraryModels() { | |||
"javax.lang.model.util.Elements", | |||
"getDocComment(javax.lang.model.element.Element)"), | |||
0) | |||
.put(methodRef("java.util.Optional", "<T>of(T)"), 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few blocking comments, but they are mostly nits about method names and docstrings. This is looking really good to me now overall 👍
"dereferenced expression " + baseExpression.toString() + " is @Nullable"; | ||
ErrorMessage errorMessage = new ErrorMessage(MessageTypes.DEREFERENCE_NULLABLE, message); | ||
|
||
handler.getErrorMessage(baseExpression, state, errorMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is changing the error message text, rather than retrieving a new copy, it shouldn't be called get[...]
. How about Handler.onPrepareErrorMessage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
GET_ON_EMPTY_OPTIONAL; | ||
} | ||
|
||
public static class ErrorMessage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this. But, can we extract this and MessageTypes
to another class/file if we are using them from the handlers. NullAway.java
is just massive. Maybe this should be a top level ErrorMessage
class containing the ErrorMessage.MessageTypes
enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍Done
} | ||
} | ||
return false; | ||
return handler.filterApForLocalVarInfoBefore(ap, state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure the name is super clear. But can't also think of a better one. @msridhar ? (This definitely is a better design, though :) )
p.s. See #278 (comment) for further discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍Done
}); | ||
} | ||
|
||
/** | ||
* @param path tree path of static method, or initializer block | ||
* @param context Javac context | ||
* @return fields guaranteed to be nonnull at exit of static method (or initializer block) | ||
* @return fields guaranteed to be nonnull at exit of static method (or initialize r block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
* | ||
* @param accessPath The access path that needs to be checked if filtered. | ||
* @param state The current visitor state. | ||
* @return true if the accesspath should be filtered to be included in the local variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"should be filtered to be included" doesn't easily parse in my mind. Are we excluding or including? (e.g. is this a whitelist or a blacklist?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the rest of the code more carefully, I'd go with "@return true if the nullability information for this accesspath should be treated as part of the surrounding context when processing a lambda expression or anonymous class declaration"
I'd also rename getLocalVarInfoBefore
to e.g. getNullnessInfoBeforeNewContext
, to more accurately reflect the fact that it preserves info for APs other than locals. But let's see if that makes sense to @msridhar as well before going ahead with that renaming for a core API method :)
If we do rename that, we can have this method be Handler.includeApInfoInSavedContext
, or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @lazaroclapp's comments and suggested names 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍Done
LGTM once @lazaroclapp's comments are addressed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit. Otherwise LGTM.
@@ -1960,7 +1961,10 @@ private Description matchDereference( | |||
* @return the error description | |||
*/ | |||
private Description createErrorDescription( | |||
MessageTypes errorType, Tree errorLocTree, String message, TreePath path) { | |||
com.uber.nullaway.ErrorMessage.MessageTypes errorType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ErrorMessage.MessageTypes
, no? Rather than the FQN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
derefExpression, | ||
message, | ||
errorMessage.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably change all the createErrorDescription[...]
methods to just take ErrorMessage
rather than the separate components. But let's leave that for a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely.
Added Optional Emptiness handler which does couple of things
optionalFoo.get()
hints that it can be Nullable.optionalFoo.isPresent()
sets theoptionalFoo.get()
access path to Non-null.This allows
and returns a warning if not implied safe.
Fixes #274
Added unit tests